pulseaudioFull: fix wrapGApp wrapping, fix vm tests by Yarny0 · Pull Request #180976 · NixOS/nixpkgs

Yarny0 · 2022-07-10T11:05:56Z

Since 7a2605e the pulseaudio build recipe incorporates the wrapGAppsHook wrapper setup-hook if advancedBluetoothCodecs are enabled. This wrapper setup-hook -- like most wrappers -- wraps binaries in $out/bin by first renaming them, then placing a wrapper script where the original binary was.

Unfortunatelly, pulseaudio doesn't like its binary moved around after installation: It records the binaries path during installation time in https://gitlab.freedesktop.org/pulseaudio/pulseaudio/-/blob/e5ad31e873eed62bc580a86a61177047f9e8c491/meson.build#L154 , then checks the path in /proc/self/exe and complains

Jun 16 19:06:48 nixosb pulseaudio[2219]: W: [.pulseaudio-wra] main.c: /proc/self/exe does not point to /nix/store/bqfyzxwpxa2ydmyvh3j32xrm4chxbj22-pulseaudio-15.0/bin/pulseaudio, cannot self execute. Are you playing games?

if they don't match: https://gitlab.freedesktop.org/pulseaudio/pulseaudio/-/blob/e5ad31e873eed62bc580a86a61177047f9e8c491/src/daemon/main.c#L577

Somehow, this also results in a real bug: pacmd fails to connect to the pulseaudio server, see #177915 .

Description of changes

To fix this issue, the commit at hand changes the installation directory for binaries to $out/.bin-unwrapped. After the installation, $out/bin is created by hand and populated with symlinks to files in $out/.bin-unwrapped. wrapGAppsHook doesn't know or care about the .bin-unwrapped directory; it just sees all the symlinks in bin, renames them and places wrapper scripts beside them.

Effectively, this leaves the original binary in .bin-unwrapped unchanged! So pulseaudio will find itself still in its oritinal place, and "users" of the package can call pulseaudio via the wrapper script in bin as usual.

Things done

More comments

I tried nixpkgs-review and it attempted a mass rebuild, which I had to abort. The mass rebuild is likely caused by this pull request also touching libpulseaudio. So it better targets staging...
I tested a minimal (e.g., non-graphical) NixOS installation with pulseaudioFull and the patch at hand applied. After some compiling, the system booted, pulseaudio started, it was able to play some music with paplay, and pacmd info also worked, i.e., the patch fixes pulseaudioFull: pacmd cannot connect to daemon #177915 .
The patch, in particular the new .bin-unwrapped directory, feel a bit unorthodox. It attempts to fix a minor bug in the current implementation and I hope it doesn't introduce new ones. I can't test this on my productive system as I cannot rebuild everything, but good testing seems to be needed here.
notifying maintainer: @lovek323
notifying bug report participant and author of the commit that introduced wrapGAppsHook: @schmittlauch

schmittlauch · 2022-07-23T12:39:08Z

Thanks for implementing your idea. I've successfully integrated your modified pulseaudio into my system, audio still works fine. But I still struggle a bit with understanding what happens there:

$ ls -lsh /nix/store/4f9zi1250bcm869kk6c3zgqpci9ysgmj-pulseaudio-15.0/.bin-unwrapped 
insgesamt 488K
 84K -r-xr-xr-x 1 root root  83K  1. Jan 1970  pacat
 44K -r-xr-xr-x 1 root root  43K  1. Jan 1970  pacmd
112K -r-xr-xr-x 1 root root 111K  1. Jan 1970  pactl
4,0K -r-xr-xr-x 1 root root 2,4K  1. Jan 1970  padsp
4,0K -r-xr-xr-x 1 root root 2,1K  1. Jan 1970  pa-info
4,0K lrwxrwxrwx 1 root root    5  1. Jan 1970  pamon -> pacat
4,0K lrwxrwxrwx 1 root root    5  1. Jan 1970  paplay -> pacat
4,0K lrwxrwxrwx 1 root root    5  1. Jan 1970  parec -> pacat
4,0K lrwxrwxrwx 1 root root    5  1. Jan 1970  parecord -> pacat
 44K -r-xr-xr-x 1 root root  44K  1. Jan 1970  pasuspender
 40K -r-xr-xr-x 1 root root  38K  1. Jan 1970  pax11publish
136K -r-xr-xr-x 1 root root 134K  1. Jan 1970  pulseaudio
4,0K -r-xr-xr-x 1 root root 2,7K  1. Jan 1970  start-pulseaudio-x11

$ ls -lsh /nix/store/4f9zi1250bcm869kk6c3zgqpci9ysgmj-pulseaudio-15.0/bin           
insgesamt 208K
16K -r-xr-xr-x 1 root root 16K  1. Jan 1970  pacat
16K -r-xr-xr-x 1 root root 16K  1. Jan 1970  pacmd
16K -r-xr-xr-x 1 root root 16K  1. Jan 1970  pactl
16K -r-xr-xr-x 1 root root 16K  1. Jan 1970  padsp
16K -r-xr-xr-x 1 root root 16K  1. Jan 1970  pa-info
16K -r-xr-xr-x 1 root root 16K  1. Jan 1970  pamon
16K -r-xr-xr-x 1 root root 16K  1. Jan 1970  paplay
16K -r-xr-xr-x 1 root root 16K  1. Jan 1970  parec
16K -r-xr-xr-x 1 root root 16K  1. Jan 1970  parecord
16K -r-xr-xr-x 1 root root 16K  1. Jan 1970  pasuspender
16K -r-xr-xr-x 1 root root 16K  1. Jan 1970  pax11publish
16K -r-xr-xr-x 1 root root 16K  1. Jan 1970  pulseaudio
16K -r-xr-xr-x 1 root root 16K  1. Jan 1970  start-pulseaudio-x11

$ file /nix/store/4f9zi1250bcm869kk6c3zgqpci9ysgmj-pulseaudio-15.0/.bin-unwrapped/pulseaudio
/nix/store/4f9zi1250bcm869kk6c3zgqpci9ysgmj-pulseaudio-15.0/.bin-unwrapped/pulseaudio: ELF 64-bit LSB executable, x86-64, version 1 (SYSV), dynamically linked, interpreter /nix/store/d2bpliayddadf6lx6l1i04w265gqw8n6-glibc-2.34-210/lib/ld-linux-x86-64.so.2, for GNU/Linux 2.6.32, with debug_info, not stripped

$ file /nix/store/4f9zi1250bcm869kk6c3zgqpci9ysgmj-pulseaudio-15.0/bin/pulseaudio           
/nix/store/4f9zi1250bcm869kk6c3zgqpci9ysgmj-pulseaudio-15.0/bin/pulseaudio: ELF 64-bit LSB executable, x86-64, version 1 (SYSV), dynamically linked, interpreter /nix/store/d2bpliayddadf6lx6l1i04w265gqw8n6-glibc-2.34-210/lib/ld-linux-x86-64.so.2, for GNU/Linux 2.6.32, not stripped

Didn't wrapGApps use to create wrapper bash scripts? Somehow both pulseaudio files in .bin-unwrapped/ as well as in bin/ are identified as ELF binaries, not scripts. Additionally, none of them is a symlink. The files do differ in size though.

Once I understand what's going on there, I additionally suggest to only enable that symlink-wrapper dance when it's actually needed, based on the same condition like for including wrapGApps.

schmittlauch · 2022-07-23T12:45:40Z

Okay, update, I now discovered that wrapGAppsHook wrappers are binary for quite a while already, thanks to makeBinaryWrapper. Let's see how long it takes until I figured out the changes in symlinking behavior, please bear with me ^_^

schmittlauch · 2022-07-23T13:50:18Z

I got it, forgot to check the hidden files:

$ ls -lshA /nix/store/080v37wdjqrcscljklimk7p6zhpqzwsj-pulseaudio-15.0/bin
total 508K
 16K -r-xr-xr-x 1 root root  16K Jan  1  1970 pacat
 64K -r-xr-xr-x 1 root root  61K Jan  1  1970 .pacat-wrapped
 16K -r-xr-xr-x 1 root root  16K Jan  1  1970 pacmd
 24K -r-xr-xr-x 1 root root  21K Jan  1  1970 .pacmd-wrapped
 16K -r-xr-xr-x 1 root root  16K Jan  1  1970 pactl
 92K -r-xr-xr-x 1 root root  90K Jan  1  1970 .pactl-wrapped
 16K -r-xr-xr-x 1 root root  16K Jan  1  1970 padsp
4,0K -r-xr-xr-x 1 root root 2,4K Jan  1  1970 .padsp-wrapped
 16K -r-xr-xr-x 1 root root  16K Jan  1  1970 pa-info
4,0K -r-xr-xr-x 1 root root 2,1K Jan  1  1970 .pa-info-wrapped
4,0K lrwxrwxrwx 1 root root    5 Jan  1  1970 pamon -> pacat
4,0K lrwxrwxrwx 1 root root    5 Jan  1  1970 paplay -> pacat
4,0K lrwxrwxrwx 1 root root    5 Jan  1  1970 parec -> pacat
4,0K lrwxrwxrwx 1 root root    5 Jan  1  1970 parecord -> pacat
 16K -r-xr-xr-x 1 root root  16K Jan  1  1970 pasuspender
 24K -r-xr-xr-x 1 root root  23K Jan  1  1970 .pasuspender-wrapped
 16K -r-xr-xr-x 1 root root  16K Jan  1  1970 pax11publish
 20K -r-xr-xr-x 1 root root  17K Jan  1  1970 .pax11publish-wrapped
 16K -r-xr-xr-x 1 root root  16K Jan  1  1970 pulseaudio
112K -r-xr-xr-x 1 root root 112K Jan  1  1970 .pulseaudio-wrapped
 16K -r-xr-xr-x 1 root root  16K Jan  1  1970 start-pulseaudio-x11
4,0K -r-xr-xr-x 1 root root 2,6K Jan  1  1970 .start-pulseaudio-x11-wrapped

But apparently we now have the same binaries in the package twice, diff /nix/store/4f9zi1250bcm869kk6c3zgqpci9ysgmj-pulseaudio-15.0/{.bin-unwrapped/pulseaudio,bin/.pulseaudio-wrapped} does not return any differences.

Can we somehow remove one of the binaries or does the whole perceived path and executable logic not add up anymore?

Yarny0 · 2022-07-23T16:54:04Z

Thanks for your feedback, @schmittlauch .

I additionally suggest to only enable that symlink-wrapper dance when it's actually needed, based on the same condition like for including wrapGApps.

This would certainly simplify the build process without wrapGApps. However, if we can assure ourselfes of the correctness of this symlink-wrapper dance, then I would rather avoid the additional complexity of optional-branches. Package size would only increase by a couple of symlinks, we avoid additional heterogenity in pulseaudio flavours, and bugs in the symlink dance would show up faster. But maybe the maintainer @lovek323 has a preference here?

I got it, forgot to check the hidden files:
$ ls -lshA /nix/store/080v37wdjqrcscljklimk7p6zhpqzwsj-pulseaudio-15.0/bin
total 508K
 16K -r-xr-xr-x 1 root root  16K Jan  1  1970 pacat
 64K -r-xr-xr-x 1 root root  61K Jan  1  1970 .pacat-wrapped
[...]
But apparently we now have the same binaries in the package twice, diff /nix/store/4f9zi1250bcm869kk6c3zgqpci9ysgmj-pulseaudio-15.0/{.bin-unwrapped/pulseaudio,bin/.pulseaudio-wrapped} does not return any differences.

Hmm, you're looking into two different packages here. In the package /nix/store/4f9zi1250bcm869kk6c3zgqpci9ysgmj-pulseaudio-15.0, the "unwrapped" binaries in bin should indeed be symlinks. diff certianly sees no differences: It follows the symlink and compares the file with itself. I can't explain why the expected symlinks in /nix/store/080v37wdjqrcscljklimk7p6zhpqzwsj-pulseaudio-15.0 are instead real/normal files. Can you provide the build recipe or otherwise explain how to build this one?

schmittlauch · 2022-08-14T16:48:24Z

The pulseaudioFull from this PR has been running on my machine for several weeks now, I could not not encounter any issues.

And regarding the seemingly-duplicated binaries, that's not an issue as well, sorry about that. The call chain for pulseaudio for example is
bin/pulseaudio ==[is a binary wrapper for]==> bin/.pulseaudio-wrapped ==[is a symlink to]==> .bin-unwrapped/pulseaudio

I just fell victim to diff following the symlinks.

So for me this looks pretty good and I encourage you to convert this to a proper PR and go through with this.

old URL returns 404

The test tries to detect the presence of pavucontrol's window by looking for the tab label "Playback". However, the OCR mechanism fails to resolve the text, possibly due to the grey background color. To fix this issue, we instead look for the name of the sound device ("Dummy Output") which gets resolved by OCR. Note: Strangely, the tab "Playback" *is* correctly resolved when the test is run in interactive mode. This might be due to the changed screen resolution, but I didn't investigate further.

According to pulseaudio(1), a system wide pulseaudio instance can only be accessed by members of the `pulse-access` group. This name seems to be hardcoded in pulseaudio -- I didn't find any switch to change it. We need to define the group so users can connect to the deamon. This commit also fixes the systemwide pulseaudio vm test: Previously, the test user `alice` was just a member of the `audio` group. This blocked access to the daemon and failed the test. The commit changes the group assignment and fixes the vm test.

Since NixOS@7a2605e the pulseaudio build recipe incorporates the `wrapGAppsHook` wrapper setup-hook if `advancedBluetoothCodecs` are enabled. This wrapper setup-hook -- like most wrappers -- wraps binaries in `$out/bin` by first renaming them, then placing a wrapper script where the original binary was. Unfortunatelly, pulseaudio doesn't like its binary moved around after installation: It records the binaries path during installation time https://gitlab.freedesktop.org/pulseaudio/pulseaudio/-/blob/e5ad31e873eed62bc580a86a61177047f9e8c491/meson.build#L154 then checks the path in `/proc/self/exe` and complains > Jun 16 19:06:48 nixosb pulseaudio[2219]: W: [.pulseaudio-wra] main.c: /proc/self/exe does not point to /nix/store/bqfyzxwpxa2ydmyvh3j32xrm4chxbj22-pulseaudio-15.0/bin/pulseaudio, cannot self execute. Are you playing games? if they don't match https://gitlab.freedesktop.org/pulseaudio/pulseaudio/-/blob/e5ad31e873eed62bc580a86a61177047f9e8c491/src/daemon/main.c#L577 Somehow, this also results in a real bug: `pacmd` fails to connect to the pulseaudio server, see NixOS#177915 To fix this issue, the commit at hand changes the installation directory for binaries to `$out/.bin-unwrapped`. After the installation, `$out/bin` is created by hand and populated with symlinks to files in `$out/.bin-unwrapped`. `wrapGAppsHook` doesn't know or care about the `.bin-unwrapped` directory; it just sees all the symlinks in `bin`, renames them and places wrapper scripts beside them. Effectively, this leaves the original binary in `.bin-unwrapped` unchanged! So pulseaudio will find itself still in its oritinal place, and "users" of the package can call pulseaudio via the wrapper script in `bin` as usual.

Note that `pacmd` only connects to user session daemons, so we have to skip it in system wide mode.

Yarny0 · 2022-08-18T15:03:30Z

Thanks, @schmittlauch , for your feedback and thorough testing!

I just rebased and updated the branch. Unfortunatelly, the branch now conflicts with other changes in staging, namely with fd031c0 .

The core commit "pulseaudioFull: fix wrapGApp wrapping" is still retained, although some shell commands have been shortened (e.g. "mkdir -p" instead of "mkdir --parents") due to feeback in another pull request ( #182360 (review) ).

While testing, I realized that all pulseaudio nixos vm tests currently fail, for various reasons. I fixed and extended the tests with more commits:

The URL to the sample file was broken.
One part of the test tries to decect the audio mixer via OCR, but this failed.
The test user wasn't member of the pulse-audio access group, which is needed to access the system-wide daemon.
I added tests for pulseaudioFull and for pacmd. This should detect further problems with pacmd.

I tested this branch, based on current nixos-unstable ( 762b003 ):

All (now four) pulseaudio vm tests pass.
I also tested audio playback with pulseaudio and pulseaudioFull on my machine (only the user daemon, not the system-wide version), and it works fine.

Due to the conflict with current staging, I simply rebased again, onto staging itself. Resolving the conflict seemed easy as I just concatenated the different preFixup scripts. However, I no longer can test the brach as it would recompile almost everything locally. If there is any way I can further test this pull request, please let me know.

Thanks again, @schmittlauch, for you help with fixing this issue and preparing this branch. I will now turn it into a proper pull request for staging.

Notifying pulseaudio maintainer: @lovek323
Notifying pulseaudio vm test maintainer: @Synthetica9
Notifying author of conflicting commit: @wegank , maybe you can verify that this branch doesn't clash with your changes.

SuperSandro2000 · 2022-08-19T12:41:19Z

nixos/tests/pulseaudio.nix

              ++ lib.optional pkgs.stdenv.isx86_64 testers.testPlay32;
          } // lib.optionalAttrs systemWide {
-            users.users.alice.extraGroups = [ "audio" ];
+            users.users.alice.extraGroups = [ "pulse-access" ];


@astro this is the exact problem we had on pulsebert

schmittlauch · 2022-08-23T18:17:59Z

Do we want to backport this to 22.05? I suggest doing so as long as a backport just works, as breaking pactl is a regression introduced into the original 22.05 release and thus worthy fixing.

SuperSandro2000 · 2022-08-23T23:07:48Z

@ofborg eval
@ofborg build pulseaudioFull

schmittlauch · 2022-08-26T08:10:32Z

I finally managed to build this on my laptop. Pulseaudio is still working fine (audio playback, device switching, Bluetooth audio) and a pacmd info now succeeds.

nixos-discourse · 2022-09-03T19:52:39Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/596

hexagonal-sun · 2022-09-13T12:34:28Z

It looks like there has been a change in pulseaudio which breaks hardware.pulseaudio.systemWide = true;. This used to work, however as @Yarny0 has said (and provided a fix for), the pulse-access group appears to be hard-coded in the pulse source (well it's a meson option but I don't think the nix expression of pulse audio allows us to change this).

If this could be merged ASAP that would be great!

ofborg bot requested a review from lovek323 July 10, 2022 11:15

Yarny0 mentioned this pull request Jul 10, 2022

pulseaudioFull: pacmd cannot connect to daemon #177915

Closed

Yarny0 force-pushed the pulseaudio-wrapper branch from b3d9386 to e854e3e Compare August 18, 2022 14:35

github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Aug 18, 2022

ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Aug 18, 2022

Yarny0 added 6 commits August 18, 2022 16:49

nixos/tests/pulseaudio: fix url to mp3 sample file

7df2bb1

old URL returns 404

nixos/tests/pulseaudio: add tests for pkgs.pulseaudioFull

78e5c0c

nixos/tests/pulseaudio: add test for pacmd

64d256a

Note that `pacmd` only connects to user session daemons, so we have to skip it in system wide mode.

Yarny0 force-pushed the pulseaudio-wrapper branch from e854e3e to 64d256a Compare August 18, 2022 14:59

Yarny0 changed the title ~~pulseaudioFull: fix wrapGApp wrapping~~ pulseaudioFull: fix wrapGApp wrapping, fix vm tests Aug 18, 2022

ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Aug 18, 2022

Yarny0 marked this pull request as ready for review August 18, 2022 15:04

wegank approved these changes Aug 18, 2022

View reviewed changes

schmittlauch approved these changes Aug 19, 2022

View reviewed changes

bobby285271 added the 12.approvals: 2 This PR was reviewed and approved by two persons. label Aug 19, 2022

SuperSandro2000 reviewed Aug 19, 2022

View reviewed changes

SuperSandro2000 approved these changes Aug 19, 2022

View reviewed changes

bobby285271 added 12.approvals: 3+ This PR was reviewed and approved by three or more persons. and removed 12.approvals: 2 This PR was reviewed and approved by two persons. labels Aug 20, 2022

ofborg bot added 10.rebuild-darwin: 11-100 This PR causes between 11 and 100 packages to rebuild on Darwin. and removed 10.rebuild-darwin: 101-500 This PR causes between 101 and 500 packages to rebuild on Darwin. labels Aug 23, 2022

schmittlauch added backport staging-22.05 and removed backport staging-22.05 labels Aug 26, 2022

schmittlauch mentioned this pull request Aug 26, 2022

[Backport staging-22.05] pulseaudioFull: fix wrapGApp wrapping, fix vm tests #188399

Closed

13 tasks

Mic92 merged commit b140a67 into NixOS:staging Sep 17, 2022

Yarny0 deleted the pulseaudio-wrapper branch September 29, 2022 18:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

pulseaudioFull: fix wrapGApp wrapping, fix vm tests#180976

pulseaudioFull: fix wrapGApp wrapping, fix vm tests#180976
Mic92 merged 6 commits intoNixOS:stagingfrom
Yarny0:pulseaudio-wrapper

Yarny0 commented Jul 10, 2022

Uh oh!

schmittlauch commented Jul 23, 2022

Uh oh!

schmittlauch commented Jul 23, 2022

Uh oh!

schmittlauch commented Jul 23, 2022

Uh oh!

Yarny0 commented Jul 23, 2022

Uh oh!

schmittlauch commented Aug 14, 2022

Uh oh!

Yarny0 commented Aug 18, 2022

Uh oh!

SuperSandro2000 Aug 19, 2022

Uh oh!

schmittlauch commented Aug 23, 2022

Uh oh!

SuperSandro2000 commented Aug 23, 2022

Uh oh!

schmittlauch commented Aug 26, 2022

Uh oh!

nixos-discourse commented Sep 3, 2022

Uh oh!

hexagonal-sun commented Sep 13, 2022 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Uh oh!

Conversation

Yarny0 commented Jul 10, 2022

Description of changes

Things done

More comments

Uh oh!

schmittlauch commented Jul 23, 2022

Uh oh!

schmittlauch commented Jul 23, 2022

Uh oh!

schmittlauch commented Jul 23, 2022

Uh oh!

Yarny0 commented Jul 23, 2022

Uh oh!

schmittlauch commented Aug 14, 2022

Uh oh!

Yarny0 commented Aug 18, 2022

Uh oh!

SuperSandro2000 Aug 19, 2022

Choose a reason for hiding this comment

Uh oh!

schmittlauch commented Aug 23, 2022

Uh oh!

SuperSandro2000 commented Aug 23, 2022

Uh oh!

schmittlauch commented Aug 26, 2022

Uh oh!

nixos-discourse commented Sep 3, 2022

Uh oh!

hexagonal-sun commented Sep 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

hexagonal-sun commented Sep 13, 2022 •

edited

Loading